Introduce VsCodeWindow to provide types for the fields we're reading/writing#3327
Introduce VsCodeWindow to provide types for the fields we're reading/writing#3327robertbrignull merged 1 commit intomainfrom
Conversation
| (window as any).CSS = { | ||
| supports: jest.fn().mockResolvedValue(false), | ||
| }; | ||
| window.CSS.supports = jest.fn().mockResolvedValue(false); |
There was a problem hiding this comment.
Is this equivalent in all cases? I think it depends on the implementation of JSDOM, but this assumes that window.CSS already exists and doesn't overwrite any other properties in window.CSS. I think that's fine here since all tests pass, but that means we are now changing the behavior of this property.
There was a problem hiding this comment.
That is a good point that it is a different behaviour from before. I forgot/missed that when I wrote up the PR description.
When I just changed the type this line was producing an error because the CSS object contains other fields so just overwriting it with { supports } wasn't satisfying the full type. That's why I changed it to just overwrite the supports field instead of the entire CSS object.
As you say, since it was passing before and also passing now, I suppose it doesn't matter. The CSS field exists already, and apparently we don't use any other fields from it because it was ok when we overwrote it to a nearly-empty object.
There was a problem hiding this comment.
I think if any of these assumptions change in the future then it'll produce an error that we'll notice, so I'm inclined to just leave this as it is and not worry about it 🤷🏼
2 participants
There are a few places where we are reading/writing extra fields to the
windowobject. I believe these fields which are provided by VS Code, but sometimes in the tests we are mocking them ourselves.Instead of using
window as anywe can define the type that we expect it to take usingdeclare. This is telling the typescript compiler that a variable exists with this given name and type and it will just believe us.Since this is just telling the compiler what type things are, it shouldn't change anything at runtime. Therefore hopefully if this compiles then it's safe 🤞🏼
We already do this in
extensions/ql-vscode/.storybook/preview.ts, though I couldn't see a way for us to share this code and avoid duplication. Althoughpreview.tscan import code fromsrc(as evidenced byVsCodeApi) it doesn't do so automatically and hence thisdeclare globaldoesn't get picked up. I'm not too concerned about it though 🤷🏼Checklist
ready-for-doc-reviewlabel there.